This seems reasonable - I am digging a bit to see why ReactDOMStackInjection.inject(); was there. @spicyj or @tomocchino might know.

It seems like based on the comments in ReactDOMStackInjection it is intentional that it gets injected both in ReactDOM and ReactServerDOM, and this condition could have stopped it from creating duplicate code. I will need to read through all the associated modules to give a verdict on this I think.

it is intentional that it gets injected both in ReactDOM and ReactServerDOM

Yes, because both ReactDOM and ReactDOMServer are Stack-based implementation. So they need to inject Stack-specific classes like ReactDOMComponent.

However ReactDOMServerStream (the one I’m looking at) does not use the Stack reconciler at all. So it doesn’t need to inject classes like ReactDOMComponent which are unused. The bloat its file size, but don’t actually end up being used.

this condition could have stopped it from creating duplicate code

The condition exists to prevent injecting twice when both ReactDOM and ReactDOMServer were used on the same page. It only mattered when we didn’t have flat bundles, as both ReactDOM and ReactDOMServer would use the same copy of the injection code. This condition isn’t useful now because both ReactDOM and ReactDOMServer ship with separate copies of this code. And regardless of that, we’re not using ReactDOM bundle anymore.

That said the condition would not have prevented the duplicate code. If a file is imported (like ReactDOMStackInjection is imported in ReactDOMServerStream), all its dependencies end up in the flat bundle, whether or not there is a boolean guard behind injection itself.

My point is that I think ReactDOMStackInjection is Stack-specific and has no effect for ReactDOMServerStream because it doesn’t use the Stack reconciler. So we end up pulling all the Stack code into a bundle that doesn’t use Stack. Since CI passed, I think this observation is correct: it was effectively dead code.

I am digging a bit to see why ReactDOMStackInjection.inject(); was there

I think most likely it was copypaste. Because the neighboring ReactDOMInjection.inject() was (and is) actually needed. I think Ben wrote that code before ReactDOMInjection and ReactStackDOMInjection were separated, and when rebasing it, Tom probably looked up a similar snippet in today’s ReactDOMServer, and thought both injections are necessary.

I’ll get this in as it seems to work but can follow up if something’s missing.

lgtm